Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't override 'connection' header if was set by the target server #260

Closed
wants to merge 3 commits into from
Closed

Don't override 'connection' header if was set by the target server #260

wants to merge 3 commits into from

Conversation

gilad61
Copy link
Contributor

@gilad61 gilad61 commented Jun 12, 2012

In http-proxy.js line 227 - if the original request didn't have a 'connection' header, and the target server set this header to any value, it will always be overridden and set to 'close'.
This proposed fix set the connection header only if not already set by the target server.

@gilad61
Copy link
Contributor Author

gilad61 commented Jul 19, 2012

Adding another fix -
If response headers are copied using the res.writeHead() function, then the copy is case sensitive. It means that eventually we may have two headers with same name but different casing, for example: x-My-Header: "a", x-my-header: "b".
HTTP headers are case insensitive, so the copy needs to be done with res.setHeader(), which overrides existing headers case-insensitively.

@indexzero
Copy link
Contributor

Could you try adding tests for this? It would be greatly appreciated.

@gilad61 gilad61 mentioned this pull request Jul 31, 2012
Closed
@gilad61 gilad61 closed this Jul 31, 2012
@gilad61
Copy link
Contributor Author

gilad61 commented Jul 31, 2012

Closing this pull request, and opening a new one instead #294 (#294)
The new pull request also contains tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gilad61 @indexzero